Replace `EventContext` fields `prev_group` and `delta_ids` with field `state_group_deltas` #15233

H-Shay · 2023-03-09T03:23:49Z

Currently the class EventContext uses the fields prev_group and delta_ids to hold information about the state group previous to the event (or the state group previous to the group before the event) and the state delta between the previous state group and the new state group. This information was used to try and optimize storing the state at the event, as if the previous state group was known one could just store the state delta between the old state and the new.

However, this way of tracking the information could be very confusing. This PR attempts to make it less confusing, and replaces the fields prev_group and delta_ids with the field state_group_deltas in the hope that this formulation is a little clearer and easier to work with.

changelog.d/15233.misc

synapse/events/snapshot.py

clokep · 2023-04-10T12:39:52Z

@H-Shay Is this pending my review? I think you had said in a meeting recently that you were going to look at this again, but want to ensure that I'm not blocking you!

clokep · 2023-05-01T15:30:40Z

We talked about this in person and @H-Shay is going to look into whether some of this code is even truly used in "real" situations or if the optimizations aren't needed. Once we figure that out we'll decide what to do with this PR.

Removing my review request for now.

H-Shay · 2023-05-16T18:53:44Z

Right so I think this is ready for another look.

For completeness, Erik and I logged how often the optimization that prev_group/state_group_deltas enables was hit and determined it was worth keeping. Thus I have attempted here to rework the PR to make the code clearer by reducing the amount of Nones flying around, better documentation, more documentation closer to the relevant code, and some encapsulation. Let me know what you think.

clokep

I think this is massively clearer without all the Nones floating around! I think most of my comments are minor refactoring.

tests/test_state.py

synapse/events/snapshot.py

Co-authored-by: Patrick Cloke <[email protected]>

clokep

It seems I never hit submit on this review. 😢 I think you've already updated a few of these things...

synapse/events/snapshot.py

docs/upgrade.md

erikjohnston · 2023-06-08T12:06:47Z

I think this is back on @H-Shay's plate?

H-Shay · 2023-06-12T18:54:59Z

Sooo I think I've sorted the backwards/forwards compatibility issue: serializing produces a dict which de-serializing then converts back into an object.
The modes of possible incompatibility then are:

The event context is serialized by old code and de-serialized by new code: to address that the new de-serialization code checks for the existence of the key "state_group_deltas" in the dict and if it doesn't exist just uses an empty dict for that field in the object it is building and ignores the presence of the old keys in the dict
The event context is serialized by the new code and de-serialized by the old code: to address this I had the new code add two "dummy" keys set to None corresponding to the old fields we removed ("prev_group" and "delta_ids") so the old de-serialize code will use those to reconstruct the object and ignore the new key "state_group_deltas" in the dict.

I think this solves the problem? Let me know if my logic seems wacky. (We might want to also add in this case that there may be a slight performance penalty until all workers and master are on > v1.87.0)

erikjohnston · 2023-06-13T08:17:54Z

I think this solves the problem? Let me know if my logic seems wacky.

Sounds correct to me!

(We might want to also add in this case that there may be a slight performance penalty until all workers and master are on > v1.87.0)

TBH I wouldn't bother

erikjohnston

Woo! 🎉

We should add a future maintenance task to remove the backwards compat hack in a couple of releases.

H-Shay · 2023-06-13T20:36:18Z

Merged #15233 into develop, thanks for the reviews.

#15777 is the tracking issue for removing the backwards compat code in the future.

@PaarthShah

Please note that this will be the last release of Synapse that is compatible with Python 3.7 and earlier. This is due to Python 3.7 now having reached End of Life; see our [deprecation policy](https://matrix-org.github.io/synapse/v1.87/deprecation_policy.html) for more details. - Pin `pydantic` to `^1.7.4` to avoid backwards-incompatible API changes from the 2.0.0 release. Resolves matrix-org#15858. Contributed by @PaarthShah. ([\matrix-org#15862](matrix-org#15862)) - Split out 2022 changes from the changelog so the rendered version in GitHub doesn't timeout as much. ([\matrix-org#15846](matrix-org#15846)) - Improve `/messages` response time by avoiding backfill when we already have messages to return. ([\matrix-org#15737](matrix-org#15737)) - Add spam checker module API for logins. ([\matrix-org#15838](matrix-org#15838)) - Fix a long-standing bug where media files were served in an unsafe manner. Contributed by @joshqou. ([\matrix-org#15680](matrix-org#15680)) - Avoid invalidating a cache that was just prefilled. ([\matrix-org#15758](matrix-org#15758)) - Fix requesting multiple keys at once over federation, related to [MSC3983](matrix-org/matrix-spec-proposals#3983). ([\matrix-org#15770](matrix-org#15770)) - Fix joining rooms through aliases where the alias server isn't a real homeserver. Contributed by @tulir @ Beeper. ([\matrix-org#15776](matrix-org#15776)) - Fix a bug in push rules handling leading to an invalid (per spec) `is_user_mention` rule sent to clients. Also fix wrong rule names for `is_user_mention` and `is_room_mention`. ([\matrix-org#15781](matrix-org#15781)) - Fix a bug introduced in 1.57.0 where the wrong table would be locked on updating database rows when using SQLite as the database backend. ([\matrix-org#15788](matrix-org#15788)) - Fix Sytest environmental variable evaluation in CI. ([\matrix-org#15804](matrix-org#15804)) - Fix forgotten rooms missing from initial sync after rejoining them. Contributed by Nico from Famedly. ([\matrix-org#15815](matrix-org#15815)) - Fix sqlite `user_filters` upgrade introduced in v1.86.0. ([\matrix-org#15817](matrix-org#15817)) - Document `looping_call()` functionality that will wait for the given function to finish before scheduling another. ([\matrix-org#15772](matrix-org#15772)) - Fix a typo in the [Admin API](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html). ([\matrix-org#15805](matrix-org#15805)) - Fix typo in MSC number in faster remote room join architecture doc. ([\matrix-org#15812](matrix-org#15812)) - Remove experimental [MSC2716](matrix-org/matrix-spec-proposals#2716) implementation to incrementally import history into existing rooms. ([\matrix-org#15748](matrix-org#15748)) - Replace `EventContext` fields `prev_group` and `delta_ids` with field `state_group_deltas`. ([\matrix-org#15233](matrix-org#15233)) - Regularly try to send transactions to other servers after they failed instead of waiting for a new event to be available before trying. ([\matrix-org#15743](matrix-org#15743)) - Fix requesting multiple keys at once over federation, related to [MSC3983](matrix-org/matrix-spec-proposals#3983). ([\matrix-org#15755](matrix-org#15755)) - Allow for the configuration of max request retries and min/max retry delays in the matrix federation client. ([\matrix-org#15783](matrix-org#15783)) - Switch from `matrix://` to `matrix-federation://` scheme for internal Synapse routing of outbound federation traffic. ([\matrix-org#15806](matrix-org#15806)) - Fix harmless exceptions being printed when running the port DB script. ([\matrix-org#15814](matrix-org#15814)) * Bump attrs from 22.2.0 to 23.1.0. ([\matrix-org#15801](matrix-org#15801)) * Bump cryptography from 40.0.2 to 41.0.1. ([\matrix-org#15800](matrix-org#15800)) * Bump ijson from 3.2.0.post0 to 3.2.1. ([\matrix-org#15802](matrix-org#15802)) * Bump phonenumbers from 8.13.13 to 8.13.14. ([\matrix-org#15798](matrix-org#15798)) * Bump ruff from 0.0.265 to 0.0.272. ([\matrix-org#15799](matrix-org#15799)) * Bump ruff from 0.0.272 to 0.0.275. ([\matrix-org#15833](matrix-org#15833)) * Bump serde_json from 1.0.96 to 1.0.97. ([\matrix-org#15797](matrix-org#15797)) * Bump serde_json from 1.0.97 to 1.0.99. ([\matrix-org#15832](matrix-org#15832)) * Bump towncrier from 22.12.0 to 23.6.0. ([\matrix-org#15831](matrix-org#15831)) * Bump types-opentracing from 2.4.10.4 to 2.4.10.5. ([\matrix-org#15830](matrix-org#15830)) * Bump types-setuptools from 67.8.0.0 to 68.0.0.0. ([\matrix-org#15835](matrix-org#15835))

H-Shay added 4 commits March 8, 2023 19:08

remove fields prev_group and delta_ids and add field state_group_deltas

bbaf807

check context.state_group_deltas for possible state delta

e89cbb6

update tests

82ea621

newsfragment

e8b8186

H-Shay requested a review from a team as a code owner March 9, 2023 03:23

H-Shay self-assigned this Mar 9, 2023

fix lints

d6e858c

H-Shay marked this pull request as draft March 9, 2023 05:35

H-Shay removed the request for review from a team March 9, 2023 05:35

H-Shay added 2 commits March 9, 2023 13:56

properly serialize/deserialize context.state_group_deltas

6486e1f

lint

430ea51

H-Shay marked this pull request as ready for review March 9, 2023 23:12

H-Shay requested a review from a team March 9, 2023 23:12

clokep reviewed Mar 14, 2023

View reviewed changes

fix changelog

59cbe10

clokep self-requested a review March 22, 2023 11:04

clokep removed their request for review May 1, 2023 15:30

H-Shay added 5 commits May 2, 2023 13:45

Merge branch 'develop' into shay/kill_prev_with_fire

1f19617

Merge branch 'develop' into shay/kill_prev_with_fire

75ad4a1

clarify state_group_deltas and create function to build them

492968c

fix mypy

60765d9

remove extraneous space

3a2ea21

H-Shay requested a review from clokep May 16, 2023 18:53

clokep reviewed May 17, 2023

View reviewed changes

H-Shay and others added 3 commits May 17, 2023 10:37

Update synapse/events/snapshot.py

10c853e

Co-authored-by: Patrick Cloke <[email protected]>

Update synapse/events/snapshot.py

0784641

Co-authored-by: Patrick Cloke <[email protected]>

refactors

91504ae

Update upgrade.md

28b025b

github-actions bot deployed to PR Documentation Preview May 17, 2023 20:00 View deployment

Update upgrade.md

6ac912a

github-actions bot deployed to PR Documentation Preview May 17, 2023 20:03 View deployment

fix stupid errors

b5ed756

github-actions bot deployed to PR Documentation Preview May 18, 2023 15:23 View deployment

H-Shay requested a review from clokep May 18, 2023 15:53

clokep reviewed May 18, 2023

View reviewed changes

formatting/doc change

77a58db

github-actions bot deployed to PR Documentation Preview May 18, 2023 17:49 View deployment

Merge branch 'develop' into shay/kill_prev_with_fire

5277a75

github-actions bot deployed to PR Documentation Preview June 6, 2023 02:30 View deployment

fix numbers

fa0a671

github-actions bot deployed to PR Documentation Preview June 6, 2023 02:33 View deployment

H-Shay requested a review from erikjohnston June 6, 2023 02:34

erikjohnston removed their request for review June 8, 2023 12:06

make serialization backwards/forwards compatible

49866ea

github-actions bot deployed to PR Documentation Preview June 12, 2023 18:08 View deployment

fix stray space upgrade.md

1b8769a

H-Shay requested a review from erikjohnston June 12, 2023 18:55

erikjohnston approved these changes Jun 13, 2023

View reviewed changes

H-Shay merged commit 553f2f5 into develop Jun 13, 2023

H-Shay deleted the shay/kill_prev_with_fire branch June 13, 2023 20:22

H-Shay mentioned this pull request Jun 13, 2023

Remove backwards compat hack from EventContext serialzation code #15777

Closed

H-Shay mentioned this pull request Aug 4, 2023

Drop backwards compat hack for event serialization #16069

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `EventContext` fields `prev_group` and `delta_ids` with field `state_group_deltas` #15233

Replace `EventContext` fields `prev_group` and `delta_ids` with field `state_group_deltas` #15233

H-Shay commented Mar 9, 2023

clokep commented Apr 10, 2023

clokep commented May 1, 2023

H-Shay commented May 16, 2023

clokep left a comment

clokep left a comment

erikjohnston commented Jun 8, 2023

H-Shay commented Jun 12, 2023 •

edited

Loading

erikjohnston commented Jun 13, 2023

erikjohnston left a comment

H-Shay commented Jun 13, 2023

Replace EventContext fields prev_group and delta_ids with field state_group_deltas #15233

Replace EventContext fields prev_group and delta_ids with field state_group_deltas #15233

Conversation

H-Shay commented Mar 9, 2023

clokep commented Apr 10, 2023

clokep commented May 1, 2023

H-Shay commented May 16, 2023

clokep left a comment

Choose a reason for hiding this comment

clokep left a comment

Choose a reason for hiding this comment

erikjohnston commented Jun 8, 2023

H-Shay commented Jun 12, 2023 • edited Loading

erikjohnston commented Jun 13, 2023

erikjohnston left a comment

Choose a reason for hiding this comment

H-Shay commented Jun 13, 2023

Replace `EventContext` fields `prev_group` and `delta_ids` with field `state_group_deltas` #15233

Replace `EventContext` fields `prev_group` and `delta_ids` with field `state_group_deltas` #15233

H-Shay commented Jun 12, 2023 •

edited

Loading